-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fx: exclude fields typo #69
Conversation
Pull Request Test Coverage Report for Build 11106744800Details
💛 - Coveralls |
I see the tests are failing because files in |
I can remove the comment entry for in all the jsons and that does fix the tests. However as I am unfamiliar with tests in general I am not sure if that wouldn't defeat the purpose a bit. |
Do not worry about writing any new tests - let's just fix the failing ones by adjusting the expected JSON data. |
Actually, alternatively, it may be a better idea (and less work) to adjust the config that is used in tests to include I think this should be just a matter of removing # tests/conftest.py
...
@pytest.fixture
def beets_config():
return deepcopy({**DEFAULT_CONFIG, "exclude_extra_fields": ["comments"]}) effectively keeping just the deepcopy @pytest.fixture
def beets_config():
return deepcopy(DEFAULT_CONFIG) |
Yes you were right, keeping only the deepcopy worked perfectly! I updated the PR. |
Amazing stuff, thank you. It's weird that CI does not catch missing The only missing thing now is a note in ## Unreleased
### Fixed
- ... |
All done and ready to merge :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - just one actually last thing - you want to rebase this off main
as I merged a PR with a fix to the workflows file where I adjusted the changelog. Also see the comment regarding formatting.
I might have butchered the rebase but after some struggeling I think it worked. |
Somehow your changes from the other PR ended up here? 😅 |
Oh yeah my bad, I rebased main in my fork to main in your repo and then rebased the PR branch on my main which already had both PRs merged. Lets see if I can fix that... |
Okay so I reverted the merge that incorporated the changes from my other PR and this should be correct now 😅 |
No worries! We do not want to update all of the dependencies here, since they will need a careful review, so it's best to do it separately. To fix this, run: git checkout main poetry.lock
poetry lock --no-update
git add -u
git commit -m 'Lock only beets dependency' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, we're good to go!
Due to a typo the config for fields to exclude was ignored.